Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Make editor's shortcut names translated on-site #99158

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Make editor's shortcut names translated on-site #99158

wants to merge 1 commit into from
+566 −546

Conversation

timothyqiu
Copy link
Member

@timothyqiu timothyqiu commented Nov 13, 2024 •
edited
Loading

When it's necessary to translate the name of a shortcut, the in-game solution is to automatically translate it when it's displayed (e.g. when the shortcut's name is displayed in a button's tooltip).

The editor code takes advantage of the fact that changing the editor language requires a restart. It translates the name once when the shortcut object is created.

Doing so leads to possible redundant double translations in the editor. It also creates inconsistencies in the way translations are done in the editor and in the game, which is not conducive to improving the translation system.

In this PR:

  • Use TTRC() instead of TTR() to mark a shortcut's name. TTRC() is just a marker and does not translate.
  • Modified the Shortcuts tab in the Editor Settings dialog so that the names of shortcuts are translated when displayed.
  • If a button with a shortcut key also has a tooltip text set, the string is changed from using TTR() to TTRC(). This allows the button to avoid duplicate text automatically, as well as double translations.

Note that the disadvantage of changing to TTRC() is that it does not support translation contexts. In theory, you still need to manually translate and change the name in NOTIFICATION_TRANSLATION_CHANGED in such case.

In editor codebase, the only place this happens is the tiles editor where we added context for Line. The shortcut is for drawing straight lines and needs to be distinguished from representing a line of text.

godot/editor/plugins/tiles/tiles_editor_plugin.cpp

Lines 313 to 316 in cb411fa

ED_SHORTCUT("tiles_editor/paint_tool", TTR("Paint"), Key::D);
ED_SHORTCUT("tiles_editor/line_tool", TTR("Line", "Tool"), Key::L);
ED_SHORTCUT("tiles_editor/rect_tool", TTR("Rect"), Key::R);
ED_SHORTCUT("tiles_editor/bucket_tool", TTR("Bucket"), Key::B);

The translation context here could actually be eliminated if the name itself was more precise. Therefore, I took the liberty of suffixing these shortcuts' names with "Tool".

godot/editor/plugins/tiles/tiles_editor_plugin.cpp

Lines 313 to 316 in 4d20b21

ED_SHORTCUT("tiles_editor/paint_tool", TTRC("Paint Tool"), Key::D);
ED_SHORTCUT("tiles_editor/line_tool", TTRC("Line Tool"), Key::L);
ED_SHORTCUT("tiles_editor/rect_tool", TTRC("Rect Tool"), Key::R);
ED_SHORTCUT("tiles_editor/bucket_tool", TTRC("Bucket Tool"), Key::B);

@timothyqiu timothyqiu added this to the 4.4 milestone Nov 13, 2024
@timothyqiu timothyqiu requested review from a team as code owners November 13, 2024 03:11
@timothyqiu timothyqiu modified the milestones: 4.4, 4.x Nov 14, 2024
@KoBeWi
Copy link
Member

KoBeWi commented Dec 11, 2024 •
edited
Loading

Looks like it doesn't work well with Shortcut tooltips:
image
Expected:
image
The tooltip text uses TTR, so it does not match with shortcut name. The tooltip seems to be important in case the button has no shortcut assigned.

@timothyqiu timothyqiu requested a review from a team as a code owner December 12, 2024 00:27
@timothyqiu
Copy link
Member Author

The tooltip text uses TTR, so it does not match with shortcut name. The tooltip seems to be important in case the button has no shortcut assigned.

Updated the PR to change related tooltip texts to use TTRC.

Technically most button's tooltip text can be changed to use TTRC, which also has the benefit of avoiding redundant double translations. But here I've only touched those related to shortcuts, let's keep the PR small 😛

@KoBeWi
Copy link
Member

KoBeWi commented Dec 12, 2024 •
edited
Loading

You missed this one (and possibly more?):

godot/editor/plugins/node_3d_editor_plugin.cpp

Line 8895 in 3877573

tool_button[TOOL_RULER]->set_shortcut(ED_SHORTCUT("spatial_editor/measure", TTR("Ruler Mode"), Key::M));

Which exposed another problem, though not directly related. When you search for a shortcut in settings dialog, it will only match original English name. Before this PR the dialog only matched the translated name. I think it should match both. We can improve it later.

EDIT:
It's the only missing shortcut. It was added 2 days ago.

@timothyqiu
Copy link
Member Author

Updated that new shortcut's name.

The matching rules when searching for shortcuts seemed relatively simple to modify, so I made the changes as well. (I had already modified the settings dialog originally to adjust the display logic of the list.)

godot/editor/editor_settings_dialog.cpp

Lines 435 to 463 in 470c47c

bool EditorSettingsDialog::_should_display_shortcut(const String &p_name, const Array &p_events, bool p_match_localized_name) const {
const Ref<InputEvent> search_ev = shortcut_search_by_event->get_event();
if (search_ev.is_valid()) {
bool event_match = false;
for (int i = 0; i < p_events.size(); ++i) {
const Ref<InputEvent> ev = p_events[i];
if (ev.is_valid() && ev->is_match(search_ev, true)) {
event_match = true;
break;
}
}
if (!event_match) {
return false;
}
}
const String &search_text = shortcut_search_box->get_text();
if (search_text.is_empty()) {
return true;
}
if (search_text.is_subsequence_ofn(p_name)) {
return true;
}
if (p_match_localized_name && search_text.is_subsequence_ofn(TTR(p_name))) {
return true;
}
return false;
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@KoBeWi KoBeWi KoBeWi approved these changes

Assignees
No one assigned
Labels
enhancement topic:editor topic:gui
Projects
None yet
Milestone
4.x
Development

Successfully merging this pull request may close these issues.
2 participants
@timothyqiu @KoBeWi